Adding asymmetric tests (from the data folder) for KLU, CuSolver, and RocSolver. by shakedregev · Pull Request #364 · ORNL/ReSolve

shakedregev · 2025-08-14T19:53:08Z

Description

Right now the code is messy and does not include asymmetric matrices in all tests. The tests are not standardized. Some test Activs200, some Activs2000. I standardized all tests and made sure each solver is tested on Activs2000 and Microgrid with 2000 IBRs. These matrices are small enough to not burden storage, but large enough to stress test the solvers.

Closes #361
Necessary to addresses #357.

Proposed changes

I added asymmetric tests and made configuration through the CLI options. I disabled iterative refinement in cases where the solution is already accurate to machine precision, to avoid deteriorating it, or hitting norm 0 vectors.

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask. We're here
to help! This is simply a reminder of what we are going to look for before
merging your code.

All tests pass. Code tested on
- CPU backend
- CUDA backend
- HIP backend
Code compiles cleanly with flags -Wall -Wpedantic -Wconversion -Wextra.
The new code follows Re::Solve style guidelines.
There are unit tests for the new code.
The new code is documented.
The feature branch is rebased with respect to the target branch.

shakedregev · 2025-08-20T16:00:17Z

Tests pass. Ready for review.

shakedregev · 2025-08-20T16:09:37Z

resolve/SystemSolver.cpp

+    // status += iterativeSolver_->resetMatrix(A_);
+    std::cout << "Refining solution with iterative refinement ...\n";


Suggested change

// status += iterativeSolver_->resetMatrix(A_);

std::cout << "Refining solution with iterative refinement ...\n";

status += iterativeSolver_->resetMatrix(A_);

Why is resetMatrix() function called here?

This was for debugging and I missed removing it. Good catch.

shakedregev

Removed some straggling comments.

tests/functionality/CMakeLists.txt

…#365) * Passing matrices and rhs to resolve_consumer. * Minor typo cuda-->hip

pelesh · 2025-08-25T06:39:47Z

@pelesh, @nkoukpaizan - it seems that any passing results we were getting with machine accuracy were pure luck. I created some matrices with c o n d ( A ) ≈ 1.5 and they still only achieve 6-8e-16. Interestingly, the SPD matrix does pass the test (just barely, it would not pass ϵ / 2 ,) even though its condition number is larger.

I believe the default tolerance for tests was set to $10 \times \epsilon$. I suggest we keep that uniform across all tests.

pelesh

I would revert all changes to the iterative solver and keep the scope of this PR strictly to adding new tests. The iterative solvers issues considered here are important but I would address them outside this PR.

shakedregev · 2025-08-25T12:52:13Z

I would revert all changes to the iterative solver and keep the scope of this PR strictly to adding new tests. The iterative solvers issues considered here are important but I would address them outside this PR.

We cannot have iterative refinement tests passing and do as you suggest. When we solve systems too accurately with the direct solve, IR fails. Iterative solvers are combined with IR here.

I addressed all relevant comments. In some cases my judgement lead me to keep things as they are.

shakedregev · 2025-08-25T14:30:24Z

We now consistently test with the ACTIVS200 series for the symmetric matrices.

shakedregev · 2025-08-25T16:32:16Z

All tests passing on all platforms.

nkoukpaizan

A few pieces of commented out code need to be removed before merging.

Tests pass on all platforms that I've tested. My only reservation is with changing the default value of conv_cond.

By the way, the orthogonalization assert when the solution is already accurate is only tripped when building in debug mode (likely chosen as default in CI).

tests/functionality/testKlu.cpp

resolve/LinSolverIterativeFGMRES.cpp

resolve/LinSolverIterativeRandFGMRES.cpp

nkoukpaizan · 2025-08-28T16:02:22Z

resolve/LinSolverIterativeRandFGMRES.cpp

    maxit_     = 100;   // default
    restart_   = 10;
-    conv_cond_ = 0; // default
+    conv_cond_ = 2; // default


Not necessary to address in this PR, but is there a rationale for setting member variable to their default values in constructors? I'd double check that it's truly the default and remove the setting of tol_, maxit_ and conv_cond.

Requests fixed, except for the default change which I believe is justified.

resolve/LinSolverIterativeFGMRES.cpp

* fix some more memory leaks and add ubsan support to `CMakeLists.txt` (#320) * Fix bug in KLU class affecting non-symmetric linear systems (#323) * Update Spack and setup CI with asan and ubsan variants (#322) * Point Spack to upstream v0.23.1. * Update Frontier modules after Spack upgrade. * Frontier build with +asan+ubsan. * Temporarily point Spack to nkoukpaizan-fork to add asan and ubsan. * +asan+ubsan in GH Actions. * CMake targets for asan and ubsan. * ~asan~ubsan Spack builds on Frontier by default. * Try running CI tests within spack build-env to address the issue of the consumer test not finding the appropriate compiler. * Alias for spack environment in CI. * Revert "Alias for spack environment in CI." This reverts commit fd263ae. * Using `${OLDPWD}` to identify Spack environment. * Add warnings flags to CMake configuration. (#325) --------- Co-authored-by: pelesh <peless@ornl.gov> * fix the means by which cmake finds the suitesparse include dir (#327) * Fixed GLU to work on non-symmetric matrices. Co-authored-by: shakedregev <shakedregev@users.noreply.github.com> * Synchronize devices after HIP functions (#336) * synchronize devices after HIP functions * Apply pre-commmit fixes * Update resolve/matrix/MatrixHandlerHip.cpp * fixed CUDA * fixed CUDA * Apply pre-commmit fixes * Update resolve/matrix/MatrixHandlerCuda.cpp * Revert "Update resolve/matrix/MatrixHandlerCuda.cpp" This reverts commit 30e41bb. * Revert "Apply pre-commmit fixes" This reverts commit 0bcd8f7. * Revert "fixed CUDA" This reverts commit 62623fa. * Revert "fixed CUDA" This reverts commit b9b249b. --------- Co-authored-by: Shaked Regev <shakedvregev@gmail.comregev@gmail.com> Co-authored-by: shakedregev <shakedregev@users.noreply.github.com> * Added the ability to reset a workspace without completely destroying it. (#343) * added method to reset workspace * changed tests * added reset workspace to CUDA * now with comments * Apply pre-commmit fixes * Update resolve/workspace/LinAlgWorkspaceCpu.cpp --------- Co-authored-by: Shaked Regev <shakedvregev@gmail.comregev@gmail.com> Co-authored-by: shakedregev <shakedregev@users.noreply.github.com> * Enforce F being 0 and remove duplicate extract factors code * added check for F!=0 * enforce F being 0 and clean up factor extraction code duplication * Apply pre-commmit fixes * Update resolve/LinSolverDirectKLU.cpp * cleaned up comments * Apply pre-commmit fixes * Update resolve/LinSolverDirectKLU.cpp --------- Co-authored-by: shakedregev <shakedregev@users.noreply.github.com> * Properly address CSR and CSC discrepancies across all solvers. Fixed discrepancies with minimal overhead. --------- Co-authored-by: shakedregev <shakedregev@users.noreply.github.com> Co-authored-by: pelesh <peless@ornl.gov> * Put sorting within KLU instead of within other solver logic. (#359) * Adding asymmetric tests (from the data folder) for KLU, CuSolver, and RocSolver. (#364) * tests that are supposed to pass are passing * fixed typos, removed unnecessary file * Apply pre-commmit fixes * renamed matrix files (not added before accidentally) * removed commented out code * fixed failing tests * Apply pre-commmit fixes * fixed commented out code * straggling comment * Update tests/functionality/CMakeLists.txt * Update tests/functionality/CMakeLists.txt * Update tests/functionality/CMakeLists.txt * fixed data path for lusol * consumer * fixed typo * changing to sys refactor, which requires a path * Fix for resolve_consumer to take in the matrices and right hand sides (#365) * Passing matrices and rhs to resolve_consumer. * Minor typo cuda-->hip * adding generic test matrices * modified numbering to suffix * tests failing with well conditioned matrix * updating to better conditioned matrices * SPD test passes consistently, others don't * still testing * changed matrices * updated matrices * changed test mats * changed matrices * symmetric test passing, asymmetric passing when run separately, but not through make test * changed matrices * changed tolerance to machine precision, tests pass! * Apply pre-commmit fixes * updated consumer * fixed non IR related comments * removed IR check, it passes with IR with the better scaled matrices * Apply pre-commmit fixes * Update tests/functionality/CMakeLists.txt * tests passing * Apply pre-commmit fixes * fixed prints * set convergence conditions * setting convergence condition, but it's overridden * set default to relative residual * added default conv_cond * Apply pre-commmit fixes * Update examples/resolve_consumer/CMakeLists.txt * addressed comment * reverted change that made CI test not pass * made matrices consistent, tolerance too stringent * tests passing, renamed files to be consistent * addressed all comments, only tested on Frontier * addressed comments * removed straggling print --------- Co-authored-by: shakedregev <shakedregev@users.noreply.github.com> Co-authored-by: Nicholson Koukpaizan <72402802+nkoukpaizan@users.noreply.github.com> * Shaked/rework examples csr (#367) * fixed examples * kalmarek example * moved to experimental, kalmarek example works * removed commented code * Apply pre-commmit fixes * Update examples/experimental/CMakeLists.txt * addressed change requests * changed cmake --------- Co-authored-by: shakedregev <shakedregev@users.noreply.github.com> * Fixed experimental examples to use CSR only. * removed unnecessary Csc reference from examples * removed build files * fixed segfault * fixed glu update * fixed all experimental examples * somehow this segmentation fault fix didn't save * fixed fgmres * Apply pre-commmit fixes * Update examples/experimental/r_KLU_cusolverrf_redo_factorization.cpp * Update examples/experimental/r_KLU_cusolverrf_redo_factorization.cpp * Update examples/experimental/r_KLU_cusolverrf_redo_factorization.cpp --------- Co-authored-by: shakedregev <shakedregev@users.noreply.github.com> * Fix warning on r_KLU_rocsolverrf_asym6x6 * Fix warning on r_KLU_rocsolverrf_asym6x6. --------- Co-authored-by: nkoukpaizan <nkoukpaizan@users.noreply.github.com> Co-authored-by: Shaked Regev <35384901+shakedregev@users.noreply.github.com> * Removed all CSC code from CSR solvers. (#375) * removed all CSC code * Apply pre-commmit fixes * Update tests/functionality/testRefactor.cpp --------- Co-authored-by: shakedregev <shakedregev@users.noreply.github.com> * Removed rocSparse "fast" solver from our options (#377) Removed all "fast" solver code. --------- Co-authored-by: shakedregev <shakedregev@users.noreply.github.com> * Updated changelog and PR template (#381) * Update emails in README.md * Add code of conduct. * Move code of conduct. * Add badges * Change action name to CPU Tests * Update README.md * Add logo to the README file. * Add logo to readthedocs. * Manage images for light/dark schemes in README. * Add logo to Doxygen docs. * Fix path to logo in Doxygen. * Update logos * Add svg logo images. * Update README.md * Update doxygen landing page. * Monor changes to Doxygen landing page * Updated coding and developer guidelines (#242) Added details about implicit coding guidelines that were not specified. Added suggested branching workflow. Added PR reviewing and opening and issue guidelines. Fixed typos Restructured documentation sections Co-authored-by: pelesh <peless@ornl.gov> * Update readthedocs links in README.md Updated links in README to pint to readthedocs pages generated from `frontmatter-dev` branch. This needs to be reverted before merging. * documentation clarification (#267) * fixing typos * fixed typos * Restructured documentation files (#299) --------- Co-authored-by: Adham Ibrahim <37982706+adham-ibrahim7@users.noreply.github.com> Co-authored-by: Slaven Peles <peless@ornl.gov> * Apply pre-commmit fixes * Use Re::Solve instead of ReSolve and update Doxygen logo. * Simplify doxygen stylesheet, allow dark mode * [skip ci] Revert sidebar-only mode. * [skip ci] Another shot on sidebar only doxy theme * ReSolve -> Re::Solve * Apply pre-commmit fixes * Copy Doxyfile.in -> Doxyfile and then modify. * updated contributors * straggling changes * Use correct Doxyfile * Undo doxyfile template changes * rebased frontmatter-dev --------- Co-authored-by: superwhiskers <whiskerdev@protonmail.com> Co-authored-by: Nicholson Koukpaizan <72402802+nkoukpaizan@users.noreply.github.com> Co-authored-by: pelesh <peless@ornl.gov> Co-authored-by: shakedregev <shakedregev@users.noreply.github.com> Co-authored-by: Shaked Regev <shakedvregev@gmail.comregev@gmail.com> Co-authored-by: nkoukpaizan <nkoukpaizan@users.noreply.github.com> Co-authored-by: Adham Ibrahim <37982706+adham-ibrahim7@users.noreply.github.com> Co-authored-by: pelesh <pelesh@users.noreply.github.com>

shakedregev added 2 commits August 14, 2025 19:47

tests that are supposed to pass are passing

3cc3ae0

fixed typos, removed unnecessary file

81fc4a4

shakedregev requested a review from pelesh August 14, 2025 19:53

shakedregev self-assigned this Aug 14, 2025

Apply pre-commmit fixes

c5b898a

shakedregev marked this pull request as draft August 14, 2025 19:53

shakedregev added 2 commits August 14, 2025 19:54

renamed matrix files (not added before accidentally)

e8d2032

removed commented out code

9d5df32

shakedregev force-pushed the shaked/asym_tests branch from c2541c4 to 9d5df32 Compare August 20, 2025 15:38

shakedregev and others added 2 commits August 20, 2025 15:54

fixed failing tests

5d6bfff

Apply pre-commmit fixes

e0ec030

shakedregev marked this pull request as ready for review August 20, 2025 15:59

shakedregev requested review from adhamsi and nkoukpaizan August 20, 2025 15:59

shakedregev changed the title ~~DRAFT: Adding asymmetric tests (from the data folder) for KLU, CuSolver, and RocSolver.~~ Adding asymmetric tests (from the data folder) for KLU, CuSolver, and RocSolver. Aug 20, 2025

fixed commented out code

2b95d98

shakedregev commented Aug 20, 2025

View reviewed changes

straggling comment

fdfafb4

shakedregev commented Aug 20, 2025

View reviewed changes

tests/functionality/CMakeLists.txt Outdated Show resolved Hide resolved

tests/functionality/CMakeLists.txt Outdated Show resolved Hide resolved

tests/functionality/CMakeLists.txt Outdated Show resolved Hide resolved

shakedregev and others added 6 commits August 20, 2025 12:14

Update tests/functionality/CMakeLists.txt

e8acb76

Update tests/functionality/CMakeLists.txt

85368b7

Update tests/functionality/CMakeLists.txt

ef6f549

fixed data path for lusol

883b88a

consumer

af9bc3f

fixed typo

65af129

shakedregev marked this pull request as draft August 20, 2025 18:52

shakedregev and others added 2 commits August 20, 2025 19:16

changing to sys refactor, which requires a path

5707b80

Fix for resolve_consumer to take in the matrices and right hand sides (…

8f08701

…#365) * Passing matrices and rhs to resolve_consumer. * Minor typo cuda-->hip

shakedregev marked this pull request as ready for review August 20, 2025 20:02

Update examples/resolve_consumer/CMakeLists.txt

45ff111

pelesh previously requested changes Aug 25, 2025

View reviewed changes

pelesh removed the request for review from adhamsi August 25, 2025 06:44

pelesh added this to the Release 0.99.2 milestone Aug 25, 2025

pelesh added enhancement New feature or request testing direct solver labels Aug 25, 2025

shakedregev added 5 commits August 25, 2025 12:55

addressed comment

ef8f5cf

reverted change that made CI test not pass

b6a4810

made matrices consistent, tolerance too stringent

3222810

tests passing, renamed files to be consistent

f5e93d1

addressed all comments, only tested on Frontier

40f90ac

shakedregev force-pushed the shaked/asym_tests branch from 2eaab1f to 40f90ac Compare August 25, 2025 14:28

shakedregev requested a review from pelesh August 25, 2025 15:08

nkoukpaizan previously requested changes Aug 28, 2025

View reviewed changes

addressed comments

9e64fd3

shakedregev requested a review from nkoukpaizan August 28, 2025 16:21

nkoukpaizan approved these changes Aug 28, 2025

View reviewed changes

resolve/LinSolverIterativeFGMRES.cpp Outdated Show resolved Hide resolved

removed straggling print

84693a9

shakedregev merged commit 962291a into develop Aug 28, 2025
6 checks passed

shakedregev deleted the shaked/asym_tests branch August 28, 2025 18:54

		// status += iterativeSolver_->resetMatrix(A_);
		std::cout << "Refining solution with iterative refinement ...\n";

	// status += iterativeSolver_->resetMatrix(A_);
	std::cout << "Refining solution with iterative refinement ...\n";
	status += iterativeSolver_->resetMatrix(A_);

Conversation

shakedregev commented Aug 14, 2025 • edited by pelesh Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Proposed changes

Checklist

Uh oh!

shakedregev commented Aug 20, 2025

Uh oh!

shakedregev Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

pelesh Aug 24, 2025

Choose a reason for hiding this comment

Uh oh!

shakedregev Aug 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shakedregev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pelesh commented Aug 25, 2025

Uh oh!

pelesh left a comment

Choose a reason for hiding this comment

Uh oh!

shakedregev commented Aug 25, 2025

Uh oh!

shakedregev commented Aug 25, 2025

Uh oh!

shakedregev commented Aug 25, 2025

Uh oh!

nkoukpaizan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nkoukpaizan Aug 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shakedregev commented Aug 14, 2025 •

edited by pelesh

Loading

shakedregev Aug 25, 2025 •

edited

Loading